Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add commands to show report, reconnect to Toggl API #149

Merged
merged 3 commits into from
Jul 14, 2024

Conversation

leoccyao
Copy link
Contributor

@leoccyao leoccyao commented Jan 2, 2024

Adds a new command "Open report view" that opens and focuses the report view leaf.

Following the same structure as liamcain/obsidian-calendar-plugin#275 (thanks for the fix!).
Closes #146
Closes #11

@leoccyao leoccyao changed the title Add command to show report Add commands to show report, reconnect to Toggl API Jan 3, 2024
@leoccyao
Copy link
Contributor Author

leoccyao commented Jan 3, 2024

Adds a new command "Refresh API Connection" to reinitiate the Toggl API connection, and also makes the status bar run a refresh on click. This is useful both for updating the sidebar report when other time entries are edited, or re-establishing a connection after a network outage.

Partially remediates #139, though does not provide a sidebar button (I personally rarely use the sidebar). It's an easy addition from here though!

@mcndt mcndt self-requested a review January 13, 2024 17:26
@mcndt mcndt added the enhancement New feature or request label Jan 13, 2024
@leoccyao
Copy link
Contributor Author

Just wondering if there's any update on when my contribution can be reviewed!

@ShyamGadde
Copy link

Hey @leoccyao, thank you for this fix. I just started using this plugin, and this issue was bothering me quite a bit. I'm not sure why this hasn't been merged yet. Anyway, I copied your code into my main.js file, and everything worked as expected.

I would like to suggest a few things, if I may. It would be nice to get the notification when using the command as well, just like when clicking the status bar item. So, I tried the following, and it did the job. However, I'm not sure if this is the right way to do it.

this.addCommand({
  checkCallback: (checking) => {
    if (!checking) {
      new Notice('Reconnecting to Toggl...')
      this.toggl.setToken(this.settings.apiToken);
    } else {
      return this.settings.apiToken != null || this.settings.apiToken != "";
    }
  },
  id: "refresh-api",
  name: "Refresh API Connection",
});

Regarding the status bar item, it would be nice to get some visual indication that it is clickable and what it does. After looking at some of the native Obsidian status bar items, I tried the following, and it behaves like a button with a tooltip.

this._plugin = plugin;
this._statusBarItem = this._plugin.addStatusBarItem();
this._statusBarItem.setText("Connecting to Toggl...");
this._plugin.registerDomEvent(
  this._statusBarItem, "click", () => {
    new Notice("Reconnecting to Toggl...")
    this.setToken(this._plugin.settings.apiToken)
  }
)
this._statusBarItem.classList.add("mod-clickable")
this._statusBarItem.style.cursor = "pointer"
this._statusBarItem.setAttribute("aria-label", "Refresh Connection")
this._statusBarItem.setAttribute("data-tooltip-position", "top")

By the way, I'm quite new to this Obsidian plugin stuff, so I don't know if I'm doing something wrong or if there's a better way to do it. I was just playing around with the main.js file, and this seemed to work for me. So, do as you see fit. I'm no expert. Thanks again. Cheers.

Copy link

@bfeitknecht bfeitknecht left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@bfeitknecht
Copy link

can you help me out how to add the fix to the main.jsfile? i'm a bit stuck

@ShyamGadde
Copy link

Hey @bs10x,

Regarding how to setup the command, you will find more this.addCommand definitions around line 26214. Adding the above snippet after those definitions should work.

For the second part, search for the TogglService class, which should be around line 20126, and modify its constructor to look like this:

constructor(plugin) {
  this._currentTimerInterval = null;
  this._currentTimeEntry = null;
  this._ApiAvailable = "UNTESTED" /* UNTESTED */;
  this._plugin = plugin;
  this._statusBarItem = this._plugin.addStatusBarItem();
  this._statusBarItem.setText("Connecting to Toggl...");
  this._plugin.registerDomEvent(
    this._statusBarItem, "click", () => {
      new Notice("Reconnecting to Toggl...");
      this.setToken(this._plugin.settings.apiToken);
    }
  );
  this._statusBarItem.classList.add("mod-clickable");
  this._statusBarItem.style.cursor = "pointer";
  this._statusBarItem.setAttribute("aria-label", "Refresh Connection");
  this._statusBarItem.setAttribute("data-tooltip-position", "top");

  togglService.set(this);
  apiStatusStore.set("UNTESTED" /* UNTESTED */);
}

Comment on lines +83 to +88
this._plugin.registerDomEvent(
this._statusBarItem, "click", () => {
new Notice('Reconnecting to Toggl...')
this.setToken(this._plugin.settings.apiToken)
}
)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great QoL improvement. One addition I would add is to set the API status to ApiStatus.UNTESTED before resetting the API client, either before calling this.setToken or at the top of this.setToken. I'll add that change in the release.

this.addCommand({
checkCallback: (checking: boolean) => {
if (!checking) {
this.toggl.setToken(this.settings.apiToken);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll also add the notice here.

Suggested change
this.toggl.setToken(this.settings.apiToken);
new Notice("Reconnecting to Toggl...");
this.toggl.setToken(this.settings.apiToken);

@mcndt mcndt merged commit 51ba08f into mcndt:master Jul 14, 2024
2 checks passed
@mcndt
Copy link
Owner

mcndt commented Jul 14, 2024

Merged! Will release this in v0.12.0. Sorry for the delays.

mcndt added a commit that referenced this pull request Jul 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
4 participants